Skip to content

[107049][Easy] Modify readme to suggest using configure #108641

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 3 commits into from
Closed

[107049][Easy] Modify readme to suggest using configure #108641

wants to merge 3 commits into from

Conversation

lionellloh
Copy link
Contributor

@lionellloh lionellloh commented Mar 2, 2023 •
edited by jyn514
Loading

This is related to #107050. Modifying instructions in readme to use configure instead of printf

Manual Test:

Command

lionellloh@lionellloh-mbp rust % ./configure --set changelog-seen=1 --set profile=user                   

configure: processing command line
configure: 
configure: changelog-seen       := 1
configure: profile              := user
configure: build.configure-args := ['--set', 'changelog-seen=1', '--set', 'profil ...
configure: 
configure: writing `config.toml` in current directory
configure: 
configure: run `python /Users/lionellloh/rust/x.py --help`

Result

16c16
< changelog-seen = 1
---
> changelog-seen = 2
26c26
< profile = user
---
> #profile = <none>
331c331
< configure-args = ['--set', 'changelog-seen=1', '--set', 'profile=user']
---
> #configure-args = []
678c678
< [target.x86_64-apple-darwin]
---
> [target.x86_64-unknown-linux-gnu]
812d811
<

@rustbot
Copy link
Collaborator

rustbot commented Mar 2, 2023

r? @Mark-Simulacrum

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added A-meta Area: Issues & PRs about the rust-lang/rust repository itself S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 2, 2023
@lionellloh lionellloh changed the title [107050][Easy] Modify readme to suggest using configure [107049][Easy] Modify readme to suggest using configure Mar 2, 2023
@albertlarsan68
Copy link
Member

I think this should be changed to suggest using ./x setup user

@lionellloh
Copy link
Contributor Author

I think this should be changed to suggest using ./x setup user

Ooh do you mind explaining why so?

I am new so just passing the message from #107049 from @jyn514

We should make it possible to set top-level keys through configure, both changelog-seen and profile. We should also update the line in the readme linked above to suggest ./configure instead of printf.

@jyn514
Copy link
Member

jyn514 commented Mar 2, 2023

@albertlarsan68 they are not quite the same. x setup user requires building bootstrap first and sometimes distros want to apply configuration when building bootstrap itself.

@jyn514
Copy link
Member

jyn514 commented Mar 2, 2023

@bors r+ rollup

@bors
Copy link
Collaborator

bors commented Mar 2, 2023

📌 Commit dccd4ec has been approved by jyn514

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Mar 2, 2023
jyn514
jyn514 suggested changes Mar 2, 2023
View reviewed changes
@jyn514
Copy link
Member

jyn514 commented Mar 2, 2023

@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 2, 2023
@albertlarsan68
Copy link
Member

Then I think there should be a section for the distros, and one for the users wanting to build from source.

@lionellloh @jyn514
Update README.md
7afe4ec 
Remove accidentally inserted line

Co-authored-by: jyn <[email protected]>
@jyn514
Copy link
Member

jyn514 commented Mar 2, 2023

I don't really want to add more blessed paths 😓 bootstrap has enough configuration knobs as-is.

What are you hoping to gain from having people run x setup instead?

jyn514
jyn514 suggested changes Mar 2, 2023
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Wait, so I read the original issue and this doesn't actually do what I want - I want ./configure to set profile=user by default, even if --set profile isn't passed.

@albertlarsan68
Copy link
Member

In one of the guides, it is recommended that x is used rather than configure, since the makefile it produces is just a wrapper around x.

@lionellloh
Copy link
Contributor Author

I think I have applied the change requested @jyn514

@jyn514
Copy link
Member

jyn514 commented Mar 4, 2023

Wait, so I read the original issue and this doesn't actually do what I want - I want ./configure to set profile=user by default, even if --set profile isn't passed.

@lionellloh I don't see this change.

@jyn514
Copy link
Member

jyn514 commented Mar 4, 2023

In one of the guides, it is recommended that x is used rather than configure, since the makefile it produces is just a wrapper around x.

Do you have a link to this? I vaguely remember some text saying "configure generates config.toml", but not a recommendation against using it.

@jyn514
Copy link
Member

jyn514 commented Apr 9, 2023

Hi @lionellloh, it's been a while - do you know when you'll have time to look at this again? Is it clear what to do?

@jyn514 jyn514 mentioned this pull request Apr 19, 2023
Merged
@jyn514
Copy link
Member

jyn514 commented Apr 19, 2023

Going to close this in favor of #110541.

@jyn514 jyn514 closed this Apr 19, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 19, 2023
@matthiaskrgr
Rollup merge of rust-lang#110541 - jyn514:fix-configure, r=ozkanonur
0820e31 
Fix various configure bugs

Fixes rust-lang#107050. Fixes rust-lang#108928. Closes rust-lang#108641.

I recommend reading this commit-by-commit to see the commit descriptions, but the code changes are small.

This also changes the README to suggest `configure` instead of `printf`, as well as a few other cleanups described in the commit message.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
1 more reviewer

@jyn514 jyn514 jyn514 requested changes

Reviewers whose approvals may not affect merge requirements
Assignees

@Mark-Simulacrum Mark-Simulacrum

Labels
A-meta Area: Issues & PRs about the rust-lang/rust repository itself S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@lionellloh @rustbot @albertlarsan68 @jyn514 @bors @Mark-Simulacrum